Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change read:me scope to profile scope #30357

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ThisIsMissEm
Copy link
Contributor

This would resolve #30355, though introduced a quirk with the scope parser logic, which I'm not sure I've solved in an acceptable manner?

Basically the profile scope would otherwise default to all which is read and write, but for profile it will only ever be read.

Screenshot 2024-05-18 at 01 31 03

@renchap renchap added this to the 4.3.0 milestone May 20, 2024
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it affect existing registrations with read:me?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file needs normalizing.

@ThisIsMissEm
Copy link
Contributor Author

How does it affect existing registrations with read:me?

There shouldn't be any yet, given it's only in bleeding-edge main, and isn't documented beyond me saying it's "coming in 4.3"

We could always do a migration on oauth_applications and oauth_tokens to rewrite read:me in scopes to profile but I'm hoping that's not necessary.

@ClearlyClaire
Copy link
Contributor

There shouldn't be any yet, given it's only in bleeding-edge main, and isn't documented beyond me saying it's "coming in 4.3"

It's been merged roughly a month ago, and two weeks ago you made it the default for new apps created through the settings interface.

Looking into mastodon.social's database, there are 4 applications with read:me as a scope.

@ClearlyClaire ClearlyClaire changed the title refactor: change read:me scope to profile scope Change read:me scope to profile scope May 21, 2024
@ClearlyClaire ClearlyClaire changed the title Change read:me scope to profile scope Change read:me scope to profile scope May 21, 2024
@ThisIsMissEm
Copy link
Contributor Author

There shouldn't be any yet, given it's only in bleeding-edge main, and isn't documented beyond me saying it's "coming in 4.3"

It's been merged roughly a month ago, and two weeks ago you made it the default for new apps created through the settings interface.

Looking into mastodon.social's database, there are 4 applications with read:me as a scope.

Oh! Good point! I'll add database migrations to this PR then; basically a find scopes LIKE %read:me% and then each in batches to rewrite?

Comment on lines +5 to +21
PROFILE_TERM = 'profile'
DEFAULT_TERM = 'all'
DEFAULT_ACCESS = %w(read write).freeze
READ_ONLY_ACCESS = %w(read).freeze

attr_reader :namespace, :term

def initialize(scope)
@namespace = scope[:namespace]&.to_s
@access = scope[:access] ? [scope[:access].to_s] : DEFAULT_ACCESS.dup
@term = scope[:term]&.to_s || DEFAULT_TERM

# override for profile scope which is read only
@access = if @term == PROFILE_TERM
READ_ONLY_ACCESS.dup
else
scope[:access] ? [scope[:access].to_s] : DEFAULT_ACCESS.dup
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may just be more readable as something like:

# override for profile scope which is read-only
@access = %w(read) if @term == 'profile'

The overhead of doing the assignment twice is negligible, and I think that special logic being self-contained in a single line is more readable than involving multiple constants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename read:me scope to "profile"
3 participants